Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permit specifying property tests as closures #62

Merged
merged 4 commits into from
Jun 27, 2018

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented Jun 25, 2018

This change permits easily sharing things captured from the environment.

Consider a simple test framework like this:

let expensive_thing = setup_expensive_thing();

let runner = TestRunner::new();
runner.test("my test", proptest! {
    |(x in 0u32..10000)| {
        expensive_thing.do_something(x)?;
    }
});

Copy link
Collaborator

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting addition that seems worthwhile. 👍

}
};

(move |($($parm:pat in $strategy:expr),+)| $body:block) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this...

|(y in 0..100)| {
assert!(x != y);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... add a test that ensures that it keeps working if we make changes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed. This one slipped my mind!

Copy link
Collaborator

@AltSysrq AltSysrq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I meant to look at this yesterday but was pretty out of it by the time it was submitted.

I do think it's worth bringing in. There's a couple simple changes I noted, but I can just do those after merging. If nothing unexpected comes up I think I can have this in tomorrow evening.

src/sugar.rs Outdated
@@ -674,6 +673,25 @@ macro_rules! proptest_helper {
(@_WRAPSTR ($a:pat, $($rest:pat),*)) => {
(stringify!($a), proptest_helper!(@_WRAPSTR ($($rest),*)))
};
// build a property testing block that when executed, executes the full property test.
(@_BODY $config:ident ($($parm:pat in $strategy:expr),+) $body:block) => {{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code that sets the source file should probably be here so that it also applies to the new case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I kept the .clone() in the calling macro to match how it behaved with clone_with_source_file().

src/sugar.rs Outdated

(|($($parm:pat in $strategy:expr),+)| $body:block) => {
|| {
let config = $crate::test_runner::Config::default();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that the config doesn't have fork() enabled since that feature requires proptest having control over the top-level test function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Ended up checking for both fork and timeout features, even though one depends on another. Possibly worth to introduce a no_fork flag in config which takes precedence to make this clearer.

@AltSysrq AltSysrq merged commit 9119682 into proptest-rs:master Jun 27, 2018
@Centril
Copy link
Collaborator

Centril commented Jun 27, 2018

PS: before you release this; I'm working on a PR related to it :)

@AltSysrq
Copy link
Collaborator

This is released in 0.8.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants